Skip to content

Conversation

@Nielsbishere
Copy link
Contributor

GSVIn doesn't permit usage of SV_PrimitiveID, even though according to the spec this is allowed.
This happens if you try to compile GS with SV_PrimitiveID with lib_ target rather than gs_.
This can break some shaders that can use IDxcLinker together with lib_.

NielsbishereAlt and others added 11 commits May 12, 2025 14:45
…t; avoids pixel shader from inverting SV_POSITION.y in a lib file
…nly supported on GS/VS/DS/MS already. Added test case for fvk-invert-y for lib files
…PrimitiveID from Vertex->Geometry shaders (GSVIn) even though according to the spec it should be allowed. This breaks existing shaders but only if they're compiled as lib file (compiling it with gs_ is fine). This is because the value in this table is NA, which results in the semantic becoming invalid and down the road that results in this error: "Semantic 'SV_PrimitiveID' is invalid as gs Input."
@Nielsbishere
Copy link
Contributor Author

Fixes #7625

@damyanp
Copy link
Member

damyanp commented Nov 4, 2025

We generally need changes to come with tests, especially if they're addressing things that should have been caught by tests but weren't.

@Nielsbishere
Copy link
Contributor Author

Yes, my bad, I've been adding them to my PRs today, but was stuck on the keep all resources one. The first PR was easy, since I could manually run dxc with the right arguments, but the D3DReflect one needed the full build test suite to run (which isn't so user friendly). This one should be easy to add a test for, will take 5 min for me.

@Nielsbishere
Copy link
Contributor Author

Nielsbishere commented Nov 4, 2025

On a second thought, this happened in a shader at my old workplace (I wasn't able to repro it as easily as I thought) . Since this monday I have no access to the source anymore, I'm asking for their help on getting the repro for the shader I lost access to. After that, I'll continue this.

@tex3d
Copy link
Contributor

tex3d commented Nov 4, 2025

This PR is not the correct fix for the issue.

This change means that SV_PrimitiveID is packed into a location in the vertex input attributes as a "system generated value". Such values would normally be loaded from those input attributes in the same way as for other signature elements. However, GS does not load the value from the vertex inputs which would use @dx.op.loadInput, it uses @dx.op.primitiveID instead.

The interpretation of "Shadow" means that there's an element included in the signature, but the value is loaded through some specialized intrinsic instead. That is the correct interpretation for SV_PrimitiveID in GS. The SigPoint location is in GSIn rather than GSVin because in HLSL it's not declared along with vertex attributes, but separately in the regular input parameters to the GS entry point.

This issue is caused by a bug when copying the input signature from the library module. In DxilSignatureElement::Initialize, we look up the semantic by name and SigPointKind, only SigPointKind was initialized from the original signature type, which is GSVIn, which causes Semantic::GetByName to return invalid for the semantic. There already is code meant to correct for this special "Shadow" element that shows up in that signature, which was called when constructing the element from metadata, but doesn't get called when copying a signature.

The bug was easy to fix once understood, so I put up a PR: #7872.

@tex3d tex3d closed this Nov 4, 2025
@github-project-automation github-project-automation bot moved this from New to Done in HLSL Roadmap Nov 4, 2025
@Nielsbishere Nielsbishere deleted the fix_sv_primitive_id_in_gsvin branch November 4, 2025 23:55
@Nielsbishere
Copy link
Contributor Author

@tex3d thank you for looking at this, and fixing it the correct way 👌 I'm still learning my way through the codebase, I did see the "Shadow" there, I just wasn't sure what it was exactly shadowing. Your explanation makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants